inlining: use method match signature for union-spliting #53600
Conversation
In cases where the results of constant inference, like concrete-eval, are used for union-split inlining, `isa`-blocks are generated using the `result.edge.specTypes` stored within each `result`. However, it's been found that the `edge` returned by abstract interpretation may have been widened by the new `@nospecializeinfer`, which can result in invalid union-splitting. To address this problem, this commit tweaks the inlining algorithm so that it performs union-split inlining using the original signatures that abstract interpretation used for union-split inference, by using `match::MethodMatch`. - fixes #53590
There was a problem hiding this comment.
LGTM. I am a bit concerned about what this means about the indiscriminate use of typeintersection results (spec_types) though. Are we being sufficiently careful to only use this result when we can prove that spec_types <: match.method.sig && spec_types <: argument to ml_matches (e.g. the exactness condition)? I would also guess that checking match.fully_covers is suitable in some cases as an alternative check, since it implies that argument <: match.method.sig && spec_types == argument, which is sufficient although not necessary.
|
Also that once the exactness condition fails on one MethodMatch in the list and so we cannot generate an exact dispatch to it, then we can only accept future method match optimizations from the list that are disjoint from the one that failed (such as concrete methods where allow_abstract=false). |
|
If it's not julia/base/compiler/ssair/inlining.jl Lines 1450 to 1452 in a182880 julia/base/compiler/ssair/inlining.jl Lines 632 to 639 in a182880 fully_covered, but that's only if there's just one single dispatch candidate.
|
In cases where the results of constant inference, like concrete-eval, are used for union-split inlining, `isa`-blocks are generated using the `result.edge.specTypes` stored within each `result`. However, it's been found that the `edge` returned by abstract interpretation may have been widened by the new `@nospecializeinfer`, which can result in invalid union-splitting. To address this problem, this commit tweaks the inlining algorithm so that it performs union-split inlining using the original signatures that abstract interpretation used for union-split inference, by using `match::MethodMatch`. - fixes #53590
In cases where the results of constant inference, like concrete-eval, are used for union-split inlining, `isa`-blocks are generated using the `result.edge.specTypes` stored within each `result`. However, it's been found that the `edge` returned by abstract interpretation may have been widened by the new `@nospecializeinfer`, which can result in invalid union-splitting. To address this problem, this commit tweaks the inlining algorithm so that it performs union-split inlining using the original signatures that abstract interpretation used for union-split inference, by using `match::MethodMatch`. - fixes #53590
|
That is not relevant to the distinction I am making. Using |
In cases where the results of constant inference, like concrete-eval, are used for union-split inlining, `isa`-blocks are generated using the `result.edge.specTypes` stored within each `result`. However, it's been found that the `edge` returned by abstract interpretation may have been widened by the new `@nospecializeinfer`, which can result in invalid union-splitting. To address this problem, this commit tweaks the inlining algorithm so that it performs union-split inlining using the original signatures that abstract interpretation used for union-split inference, by using `match::MethodMatch`. - fixes #53590
As Jameson pointed out in the link below, while the union-split handles cases when there are uncovered matches, sometimes the expected condition `spec_types <: method.sig` that the union-split algorithm relies on isn't met in such cases, which caused issues like #52644. This commit fixes the problem by adding explicit checks for such cases. Note that this is based on #52092. The extra handling for single method match unmatched static parameters based on `only_method` that was removed in JuliaLang/#52092 has been ineffective and would otherwise cause problematic inlining on this PR. We'll likely need to come back to this later and figure out a safe and effective way to deal with such cases in the future when the handling for either case turns out to be necessary. - closes #52644 - xref: <#53600 (review)>
As Jameson pointed out in the link below, while the union-split handles cases when there are uncovered matches, sometimes the expected condition `spec_types <: method.sig` that the union-split algorithm relies on isn't met in such cases, which caused issues like #52644. This commit fixes the problem by adding explicit checks for such cases. Note that this is based on #52092. The extra handling for single method match unmatched static parameters based on `only_method` that was removed in JuliaLang/#52092 has been ineffective and would otherwise cause problematic inlining on this PR. We'll likely need to come back to this later and figure out a safe and effective way to deal with such cases in the future when the handling for either case turns out to be necessary. - closes #52644 - xref: <#53600 (review)>
) As Jameson pointed out in the link below, while the union-split handles cases when there are uncovered matches, sometimes the expected condition `spec_types <: method.sig` that the union-split algorithm relies on isn't met in such cases, which caused issues like #52644. This commit fixes the problem by adding explicit checks for such cases. Note that this is based on #52092. The extra handling for single method match unmatched static parameters based on `only_method` that was removed in JuliaLang/#52092 has been ineffective and would otherwise cause problematic inlining on this PR. We'll likely need to come back to this later and figure out a safe and effective way to deal with such cases in the future when the handling for either case turns out to be necessary. - closes #52644 - xref: <#53600 (review)>
) As Jameson pointed out in the link below, while the union-split handles cases when there are uncovered matches, sometimes the expected condition `spec_types <: method.sig` that the union-split algorithm relies on isn't met in such cases, which caused issues like #52644. This commit fixes the problem by adding explicit checks for such cases. Note that this is based on #52092. The extra handling for single method match unmatched static parameters based on `only_method` that was removed in JuliaLang/#52092 has been ineffective and would otherwise cause problematic inlining on this PR. We'll likely need to come back to this later and figure out a safe and effective way to deal with such cases in the future when the handling for either case turns out to be necessary. - closes #52644 - xref: <#53600 (review)>
) As Jameson pointed out in the link below, while the union-split handles cases when there are uncovered matches, sometimes the expected condition `spec_types <: method.sig` that the union-split algorithm relies on isn't met in such cases, which caused issues like #52644. This commit fixes the problem by adding explicit checks for such cases. Note that this is based on #52092. The extra handling for single method match unmatched static parameters based on `only_method` that was removed in JuliaLang/#52092 has been ineffective and would otherwise cause problematic inlining on this PR. We'll likely need to come back to this later and figure out a safe and effective way to deal with such cases in the future when the handling for either case turns out to be necessary. - closes #52644 - xref: <#53600 (review)> (cherry picked from commit b730d33)
KristofferC
removed
the
backport 1.11
) In cases where the results of constant inference, like concrete-eval, are used for union-split inlining, `isa`-blocks are generated using the `result.edge.specTypes` stored within each `result`. However, it's been found that the `edge` returned by abstract interpretation may have been widened by the new `@nospecializeinfer`, which can result in invalid union-splitting. To address this problem, this commit tweaks the inlining algorithm so that it performs union-split inlining using the original signatures that abstract interpretation used for union-split inference, by using `match::MethodMatch`. - fixes JuliaLang#53590
In cases where the results of constant inference, like concrete-eval, are used for union-split inlining,
isa-blocks are generated using theresult.edge.specTypesstored within eachresult. However, it's been found that theedgereturned by abstract interpretation may have been widened by the new@nospecializeinfer, which can result in invalid union-splitting. To address this problem, this commit tweaks the inlining algorithm so that it performs union-split inlining using the original signatures that abstract interpretation used for union-split inference, by usingmatch::MethodMatch.